Skip to content

Fix theme toggle issue on "How it works " page and Implement Cache Busting#259

Open
Rudra-rps wants to merge 10 commits intoOWASP-BLT:mainfrom
Rudra-rps:feat/theme_toggle_how_it_works
Open

Fix theme toggle issue on "How it works " page and Implement Cache Busting#259
Rudra-rps wants to merge 10 commits intoOWASP-BLT:mainfrom
Rudra-rps:feat/theme_toggle_how_it_works

Conversation

@Rudra-rps
Copy link
Copy Markdown
Contributor

@Rudra-rps Rudra-rps commented Feb 26, 2026

Fixes layout and theme toggle on "How it works page"

Summary by CodeRabbit

  • New Features

    • Implemented asset versioning for optimized caching and faster deployments.
    • Enhanced interactive score calculator with real-time computation.
  • Bug Fixes

    • Improved error reporting with automatic sanitization of sensitive information.
  • Refactor

    • Reorganized theme system into a shared reusable module.
    • Updated service worker for better local development handling.

@github-actions
Copy link
Copy Markdown

🍃 PR Readiness Check

Check the readiness of this PR on Leaf:
👉 Open on Leaf

Leaf reviews pull requests for operational readiness, security risks, and production-impacting changes before they ship.

@github-actions github-actions bot added the files-changed: 3 PR changes 3 files label Feb 26, 2026
@Rudra-rps
Copy link
Copy Markdown
Contributor Author

image Before image

After

@Rudra-rps Rudra-rps marked this pull request as ready for review February 26, 2026 18:51
@Rudra-rps Rudra-rps marked this pull request as draft February 26, 2026 18:52
@Rudra-rps Rudra-rps force-pushed the feat/theme_toggle_how_it_works branch from 3c46985 to be92b7d Compare February 26, 2026 18:59
@DonnieBLT
Copy link
Copy Markdown
Contributor

is it ready?

@Rudra-rps
Copy link
Copy Markdown
Contributor Author

is it ready?

I was also working on the layout, specifically fixing an issue where the PR section was overflowing relative to the repository section in the current version.

image

This is the current version

image

This is the updated layout. However, I’ve noticed that the footer is occupying too much space, so I’m considering whether I should make adjustments to the footer as well.

image

@DonnieBLT
Copy link
Copy Markdown
Contributor

I have a similar update PR for the layout that uses scroll-able divs see if you like that one and I'll merge it

@Rudra-rps Rudra-rps force-pushed the feat/theme_toggle_how_it_works branch from be92b7d to 221a952 Compare February 28, 2026 19:35
@github-actions github-actions bot added files-changed: 7 PR changes 7 files and removed files-changed: 3 PR changes 3 files labels Feb 28, 2026
@Rudra-rps
Copy link
Copy Markdown
Contributor Author

Before
image

After

image

the root cause was not logic related but due to wrangler dev serving a cached version of theme.js (Cf-Cache-Status: HIT).

Solution Implemented

Disabled caching for static assets in local development (Cache-Control: no-store)

Verified fresh asset delivery (Cf-Cache-Status: MISS)

Confirmed event listeners attach correctly

Ensured theme toggle works consistently across pages

@Rudra-rps Rudra-rps marked this pull request as ready for review February 28, 2026 19:40
@Rudra-rps
Copy link
Copy Markdown
Contributor Author

I have a similar update PR for the layout that uses scroll-able divs see if you like that one and I'll merge it

Looks good to me..

@Rudra-rps
Copy link
Copy Markdown
Contributor Author

I have a similar update PR for the layout that uses scroll-able divs see if you like that one and I'll merge it

Looks good to me..

I also got another layout
Screenshot 2026-02-27 210811

Maybe something like this would work too

Copy link
Copy Markdown
Contributor

@e-esakman e-esakman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please resolve the conflicts

@Rudra-rps
Copy link
Copy Markdown
Contributor Author

please resolve the conflicts

done..

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Updates frontend theme handling and mitigates stale-asset issues (especially in local dev) for the BLT-Leaf static UI served via the Cloudflare Worker.

Changes:

  • Adds shared public/theme.js and migrates theme toggle markup to data- attributes.
  • Updates How It Works page to use the shared theme script and removes page-specific theme logic.
  • Adds local-dev cache-bypass behavior in the service worker and introduces explicit cache headers when serving assets from env.ASSETS.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
src/index.py Adds helper to serve static assets with explicit Cache-Control behavior.
public/theme.js Introduces shared theme initialization + toggle/icon behavior.
public/index.html Switches theme toggles to data- attributes and loads theme.js; adjusts SW handling in local dev.
public/how-it-works.html Removes inline theme init/toggle logic and loads shared theme.js.
public/sw.js Adds local-dev behavior to skip/bypass SW caching for static assets.
public/diagnostics.html Migrates theme toggle selectors to data- attributes.
public/test-error.html Migrates theme toggle selectors to data- attributes.
Comments suppressed due to low confidence (1)

public/how-it-works.html:14

  • theme.js is loaded after https://cdn.tailwindcss.com and after initial rendering, but it’s currently responsible for setting window.tailwind.config.darkMode and applying the initial theme. Tailwind CDN reads window.tailwind.config at load time, so setting it after Tailwind loads can make dark: variants unreliable, and applying the theme at the end of the body can cause a flash of incorrect theme. Consider restoring an early head snippet (before Tailwind loads) to set darkMode: 'class' and apply the initial dark class, and keep theme.js for the toggle/icon wiring.
    <script src="error-reporter.js"></script>

    <script src="https://cdn.tailwindcss.com"></script>
    <link rel="stylesheet" href="https://cdnjs.cloudflare.com/ajax/libs/font-awesome/6.4.0/css/all.min.css"

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@Rudra-rps
Copy link
Copy Markdown
Contributor Author

Addressed all review comments:

Split cache policy by asset type (HTML & sw.js use no-cache, must-revalidate; fingerprinted JS/CSS use long-lived immutable caching).

Fixed theme.js to respect prefers-color-scheme when no saved theme exists.

Wrapped DOM-dependent logic in DOMContentLoaded to ensure safe execution when loaded in .

Please let me know if any further adjustments are needed.

@Nachiket-Roy
Copy link
Copy Markdown
Contributor

I’m not sure about this approach. It feels more complex than necessary and could likely be simplified.
BLT-Pages handles dark mode much more efficiently with just a few lines of code.

@Rudra-rps
Copy link
Copy Markdown
Contributor Author

I’m not sure about this approach. It feels more complex than necessary and could likely be simplified. BLT-Pages handles dark mode much more efficiently with just a few lines of code.

The main issue I was addressing here was related to caching behaviour on the cloudflare worker side, which was preventing new deployments from being picked up correctly. If there is a simpler approach I’d be happy to align with it..

@Nachiket-Roy
Copy link
Copy Markdown
Contributor

Nachiket-Roy commented Mar 5, 2026

@Rudra-rps i looked into this. The problem is with multiple cache no coordination but we need this multiple cache. I got a solution as Cache Busting could you look into this if this helps?

@Rudra-rps
Copy link
Copy Markdown
Contributor Author

@Rudra-rps i looked into this. The problem is with multiple cache no coordination but we need this multiple cache. I got a solution as Cache Busting could you look into this if this helps?

Makes sense, I'll look into this. Thanks!

@github-actions github-actions bot added files-changed: 14 PR changes 14 files and removed files-changed: 7 PR changes 7 files labels Mar 6, 2026
@Rudra-rps
Copy link
Copy Markdown
Contributor Author

Rudra-rps commented Mar 6, 2026

This change introduces a lightweight fingerprinting step that hashes JS assets and rewrites the HTML references to the hashed filenames. Because the filename changes whenever the content changes, browsers and CDNs automatically fetch the new version while safely caching the old one.

Also updated the worker and service worker logic so that only fingerprinted assets receive long-lived immutable caching, while HTML and non fingerprinted assets continue to revalidate normally.

@Nachiket-Roy @DonnieBLT

@Rudra-rps Rudra-rps changed the title Fix layout height/scroll issue and resolve theme toggle on How It Wor… Fix theme toggle issue on "How it works " page and Implement Cache Busting Mar 7, 2026
@Rudra-rps
Copy link
Copy Markdown
Contributor Author

Ready for review @DonnieBLT

@owasp-blt owasp-blt bot added the has-peer-review PR has received peer review label Mar 19, 2026
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 19, 2026

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

This pull request introduces a comprehensive asset fingerprinting and caching system. It adds a build-time fingerprinting script that generates hash-based filenames for static assets, updates the build pipeline and HTML files to reference these fingerprinted versions, refactors theme management into a shared versioned module, enhances the service worker for local development, implements new backend cache control headers, adds error reporting sanitization, and introduces an interactive calculator for the how-it-works page.

Changes

Cohort / File(s) Summary
Build & Fingerprinting Infrastructure
package.json, wrangler.toml, scripts/fingerprint.js
Added fingerprinting npm script and updated build process to run fingerprint.js before compilation/dev, generating cache-busting hashed filenames and updating HTML asset references.
Asset Manifest & Configuration
public/asset-manifest.json
New manifest file mapping original asset filenames to their fingerprinted versions for reference by build and backend systems.
HTML Pages with Updated Asset References
public/index.html, public/how-it-works.html, public/diagnostics.html, public/test-error.html
Updated all HTML files to reference fingerprinted asset versions, replaced id-based theme toggle selectors with data- attributes, and removed inline theme initialization logic.
Shared Theme Module
public/theme.196e3e21.js, public/theme.js
Added new shared, versioned theme utility module handling Tailwind dark mode configuration, theme persistence to localStorage, and synchronized icon updates via data attributes.
Interactive Calculator
public/how-it-works.3ebd3397.js
Introduced new versioned calculator module for the how-it-works page that computes real-time CI/Review/Response scores with confidence badges and an overall merge-readiness status.
Error Reporting
public/error-reporter.js, public/error-reporter.1b9dc33f.js
Enhanced error reporter with sanitization helpers (truncation, secret redaction), improved URL normalization, refined resource error suppression logic, and updated console.error hook; also added fingerprinted version.
Service Worker Updates
public/sw.js
Modified service worker to detect local development mode, skip precaching and force network-first bypass for certain paths in local dev, introduce fingerprinted asset caching support, and remove precaching of non-fingerprinted how-it-works.js.
Backend Cache & Asset Handling
src/index.py
Refactored static asset handling with new cache header logic that detects fingerprinted immutable assets (returning long-lived cache) vs. non-fingerprinted assets (using no-cache), forces no-store for local dev requests, and added new /api/test-error POST endpoint alongside existing error reporting.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Suggested labels

quality: high

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 15.22% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title addresses two main changes: fixing theme toggle on 'How it works' page and implementing cache busting via fingerprinting. Both are substantive changes covered in the PR objectives and file modifications.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
public/diagnostics.html (1)

171-181: ⚠️ Potential issue | 🟡 Minor

Add null checks before accessing themeToggle and themeIcon.

Same issue as in test-error.html: if these elements aren't found, querySelector returns null, causing a TypeError on addEventListener.

🛡️ Proposed fix to add defensive null checks
 const themeToggle = document.querySelector('[data-theme-toggle]');
 const themeIcon = document.querySelector('[data-theme-icon]');
-themeToggle.addEventListener('click', () => {
-    const isDark = document.documentElement.classList.toggle('dark');
-    themeIcon.className = isDark ? 'fas fa-sun' : 'fas fa-moon';
-    localStorage.setItem('theme', isDark ? 'dark' : 'light');
-});
-// Set initial icon
-if (document.documentElement.classList.contains('dark')) {
-    themeIcon.className = 'fas fa-sun';
+if (themeToggle && themeIcon) {
+    themeToggle.addEventListener('click', () => {
+        const isDark = document.documentElement.classList.toggle('dark');
+        themeIcon.className = isDark ? 'fas fa-sun' : 'fas fa-moon';
+        localStorage.setItem('theme', isDark ? 'dark' : 'light');
+    });
+    if (document.documentElement.classList.contains('dark')) {
+        themeIcon.className = 'fas fa-sun';
+    }
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@public/diagnostics.html` around lines 171 - 181, The code assumes themeToggle
and themeIcon are non-null; add defensive null checks around their usage: after
const themeToggle = document.querySelector('[data-theme-toggle]') and const
themeIcon = document.querySelector('[data-theme-icon]'), verify both are truthy
before calling themeToggle.addEventListener, accessing themeIcon.className, or
setting the initial icon with
document.documentElement.classList.contains('dark'); if either is missing, skip
the event wiring and initial icon assignment (or optionally log a warning).
Ensure the checks reference the existing symbols themeToggle and themeIcon and
guard all subsequent accesses to those variables.
public/test-error.html (1)

225-231: ⚠️ Potential issue | 🟡 Minor

Add null checks before accessing themeToggle and themeIcon.

If the elements with [data-theme-toggle] or [data-theme-icon] are not found (e.g., due to DOM manipulation or future template changes), querySelector returns null, and calling addEventListener on null will throw a TypeError.

🛡️ Proposed fix to add defensive null checks
 const themeToggle = document.querySelector('[data-theme-toggle]');
 const themeIcon = document.querySelector('[data-theme-icon]');
-themeToggle.addEventListener('click', () => {
-    const isDark = document.documentElement.classList.toggle('dark');
-    themeIcon.className = isDark ? 'fas fa-sun' : 'fas fa-moon';
-    localStorage.setItem('theme', isDark ? 'dark' : 'light');
-});
-if (document.documentElement.classList.contains('dark')) {
-    themeIcon.className = 'fas fa-sun';
+if (themeToggle && themeIcon) {
+    themeToggle.addEventListener('click', () => {
+        const isDark = document.documentElement.classList.toggle('dark');
+        themeIcon.className = isDark ? 'fas fa-sun' : 'fas fa-moon';
+        localStorage.setItem('theme', isDark ? 'dark' : 'light');
+    });
+    if (document.documentElement.classList.contains('dark')) {
+        themeIcon.className = 'fas fa-sun';
+    }
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@public/test-error.html` around lines 225 - 231, The current code assumes
document.querySelector found elements and will throw if null; update the block
that references themeToggle and themeIcon to first guard their existence (check
themeToggle and themeIcon are truthy) before calling
themeToggle.addEventListener or setting themeIcon.className and localStorage;
alternatively use optional chaining when adding the listener and when updating
themeIcon (e.g., only call document.documentElement.classList.toggle and
localStorage.setItem inside the guarded branch) so themeToggle and themeIcon are
not accessed when null to prevent a TypeError.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@public/error-reporter.1ba0a9b8.js`:
- Around line 73-97: The current hookConsoleError wrapper forwards every
console.error invocation (console.error) by serializing arbitrary args and
calling reportError, which can leak user/request data and duplicate handled
errors; update hookConsoleError so it only calls reportError for real Error
instances found in arguments (inspect arguments for instanceof Error), avoid
JSON.stringify on plain objects (do not serialize objects or strip/replace
sensitive fields like request, headers, user, cookies), redact PII from
error.message/stack and truncate safely, and add a marker/metadata (e.g.,
source: 'console.error:unhandled') so downstream slack_notifier handlers can
skip duplicates; ensure reportError is not invoked for non-Error diagnostics to
prevent noisy Slack alerts and leakage.

In `@public/index.html`:
- Around line 332-335: The theme toggle button using the data-theme-toggle
attribute is an icon-only control with no accessible name; add an accessible
name (e.g., aria-label="Toggle theme") to the button element and mark the inner
<i> with data-theme-icon as decorative (aria-hidden="true") so assistive tech
ignores the icon; apply the same change to the other theme toggle instance (the
second data-theme-toggle / data-theme-icon pair) to ensure both buttons are
accessible.
- Line 3291: The page throws a ReferenceError because the inline app script
calls initTheme() before the theme bundle is loaded; either move the theme
script tag that loads public/theme.196e3e21.js so it appears before the inline
app script, or remove the initTheme() invocation from the inline script (or
replace it with a safe feature check like if (typeof initTheme === 'function')
initTheme()); update the inline script that contains initTheme() call and the
script tag loading public/theme.196e3e21.js to ensure initTheme() is defined
before being invoked.

In `@scripts/fingerprint.js`:
- Around line 56-61: The loop over ASSETS in fingerprint.js currently warns and
continues when a declared asset is missing; change this to fail the build
instead by throwing an error or exiting with non‑zero status when
fs.existsSync(srcPath) is false. Locate the for (const asset of ASSETS) loop and
replace the console.warn/continue branch with a console.error (including asset
and srcPath) followed by throwing a new Error or calling process.exit(1) so
missing entries in ASSETS (and PUBLIC/srcPath) cause the build to fail.

---

Outside diff comments:
In `@public/diagnostics.html`:
- Around line 171-181: The code assumes themeToggle and themeIcon are non-null;
add defensive null checks around their usage: after const themeToggle =
document.querySelector('[data-theme-toggle]') and const themeIcon =
document.querySelector('[data-theme-icon]'), verify both are truthy before
calling themeToggle.addEventListener, accessing themeIcon.className, or setting
the initial icon with document.documentElement.classList.contains('dark'); if
either is missing, skip the event wiring and initial icon assignment (or
optionally log a warning). Ensure the checks reference the existing symbols
themeToggle and themeIcon and guard all subsequent accesses to those variables.

In `@public/test-error.html`:
- Around line 225-231: The current code assumes document.querySelector found
elements and will throw if null; update the block that references themeToggle
and themeIcon to first guard their existence (check themeToggle and themeIcon
are truthy) before calling themeToggle.addEventListener or setting
themeIcon.className and localStorage; alternatively use optional chaining when
adding the listener and when updating themeIcon (e.g., only call
document.documentElement.classList.toggle and localStorage.setItem inside the
guarded branch) so themeToggle and themeIcon are not accessed when null to
prevent a TypeError.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository: OWASP-BLT/coderabbit/.coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: 4d880c58-76ae-4bad-b0cd-0f20264832b8

📥 Commits

Reviewing files that changed from the base of the PR and between 4b3d830 and b9d5f9c.

📒 Files selected for processing (14)
  • package.json
  • public/asset-manifest.json
  • public/diagnostics.html
  • public/error-reporter.1ba0a9b8.js
  • public/how-it-works.3ebd3397.js
  • public/how-it-works.html
  • public/index.html
  • public/sw.js
  • public/test-error.html
  • public/theme.196e3e21.js
  • public/theme.js
  • scripts/fingerprint.js
  • src/index.py
  • wrangler.toml

@github-actions github-actions bot added files-changed: 15 PR changes 15 files and removed files-changed: 14 PR changes 14 files labels Mar 19, 2026
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (2)
public/diagnostics.html (1)

171-180: Prefer the shared theme bundle here instead of duplicating the toggle logic.

public/theme.js already owns the same selectors and icon initialization, and scripts/fingerprint.js is set up to rewrite theme.js references for this page. Keeping a second page-local implementation here makes future theme fixes easy to miss on diagnostics.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@public/diagnostics.html` around lines 171 - 180, This page duplicates the
theme toggle logic (the DOM selectors themeToggle and themeIcon, toggling
document.documentElement.classList, updating themeIcon.className, and writing
localStorage) that already lives in the shared public/theme.js; remove this
local block and rely on the shared bundle instead — delete the click handler and
initial icon setting here and ensure the page includes/uses the existing
public/theme.js (scripts/fingerprint.js will rewrite references) so all theme
behavior is sourced from the single implementation.
scripts/fingerprint.js (1)

25-31: CSS fingerprinting is only half implemented right now.

The file header/comments describe JS/CSS support, but the HTML rewrite only matches src="...". If a stylesheet is ever added to ASSETS, the build will create the hashed file while the HTML keeps pointing at the old href. Either extend the matcher to cover href too or narrow the script/docs to JS-only.

🔧 One way to cover both `src` and `href`
-        const re = new RegExp(
-            `(src=["'][^"']*)${escapeRegex(stem)}(?:\\.[0-9a-f]{8})?${escapeRegex(ext)}(["'])`,
-            'g'
-        );
+        const re = new RegExp(
+            `((?:src|href)=["'][^"']*)${escapeRegex(stem)}(?:\\.[0-9a-f]{8})?${escapeRegex(ext)}((?:\\?[^"']*)?["'])`,
+            'g'
+        );

Also applies to: 116-124

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@scripts/fingerprint.js` around lines 25 - 31, The comment/behavior mismatch
comes from ASSETS claiming JS/CSS support while the HTML rewrite only replaces
src="..." occurrences; update the HTML rewrite logic that currently targets src
attributes so it also matches href attributes (or both src|href) when rewriting
filenames from ASSETS, ensuring stylesheet links get their fingerprinted
filenames, and keep the ASSETS array as the single source of truth;
alternatively, if you prefer JS-only, change the header comment to say "JS
assets only" instead of "JS/CSS". Reference: ASSETS and the HTML rewrite that
matches src="...".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@public/error-reporter.js`:
- Around line 97-104: The current redaction/truncation is only applied in the
console.error branch; move normalization into reportError so all reporting paths
(console error, window error, unhandledrejection) are sanitized centrally.
Update reportError to run redactSensitive and safeTruncate on its main string
parameters and on any values inside the extra payload before sending to the
server, ensuring message, stack, url, resource and other string fields are
normalized; then re-run fingerprinting so the hashed bundle stays in sync.
Ensure callers like the console handler and the handlers for
window.error/unhandledrejection simply pass raw data to reportError and do not
duplicate sanitization.

---

Nitpick comments:
In `@public/diagnostics.html`:
- Around line 171-180: This page duplicates the theme toggle logic (the DOM
selectors themeToggle and themeIcon, toggling
document.documentElement.classList, updating themeIcon.className, and writing
localStorage) that already lives in the shared public/theme.js; remove this
local block and rely on the shared bundle instead — delete the click handler and
initial icon setting here and ensure the page includes/uses the existing
public/theme.js (scripts/fingerprint.js will rewrite references) so all theme
behavior is sourced from the single implementation.

In `@scripts/fingerprint.js`:
- Around line 25-31: The comment/behavior mismatch comes from ASSETS claiming
JS/CSS support while the HTML rewrite only replaces src="..." occurrences;
update the HTML rewrite logic that currently targets src attributes so it also
matches href attributes (or both src|href) when rewriting filenames from ASSETS,
ensuring stylesheet links get their fingerprinted filenames, and keep the ASSETS
array as the single source of truth; alternatively, if you prefer JS-only,
change the header comment to say "JS assets only" instead of "JS/CSS".
Reference: ASSETS and the HTML rewrite that matches src="...".
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository: OWASP-BLT/coderabbit/.coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: 26aa1a1f-3c69-4ab5-a3f2-683edf417c36

📥 Commits

Reviewing files that changed from the base of the PR and between b9d5f9c and 6ff0a55.

📒 Files selected for processing (8)
  • public/asset-manifest.json
  • public/diagnostics.html
  • public/error-reporter.71a8b085.js
  • public/error-reporter.js
  • public/how-it-works.html
  • public/index.html
  • public/test-error.html
  • scripts/fingerprint.js
✅ Files skipped from review due to trivial changes (2)
  • public/asset-manifest.json
  • public/test-error.html
🚧 Files skipped from review as they are similar to previous changes (1)
  • public/how-it-works.html

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

♻️ Duplicate comments (1)
public/diagnostics.html (1)

58-61: ⚠️ Potential issue | 🟡 Minor

Match the accessibility fix on the diagnostics toggle.

This is still an icon-only button with only a title, so screen readers won't get a reliable name. public/index.html already applies the right pattern to the same control.

🔧 Suggested change
-                <button data-theme-toggle
+                <button data-theme-toggle aria-label="Toggle theme"
                     class="rounded-lg p-2 text-slate-500 hover:bg-slate-100 dark:text-slate-400 dark:hover:bg-slate-700 transition-colors"
                     title="Toggle dark/light mode">
-                    <i class="fas fa-moon" data-theme-icon></i>
+                    <i class="fas fa-moon" data-theme-icon aria-hidden="true"></i>
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@public/diagnostics.html` around lines 58 - 61, The diagnostics theme toggle
(element with data-theme-toggle and its icon data-theme-icon) is an icon-only
button without an accessible name; update it to match the other toggle pattern
by adding an explicit accessible label (for example add a visually-hidden <span>
with the text "Toggle dark/light mode" inside the button or add
aria-label="Toggle dark/light mode" on the button) so screen readers get a
reliable name; ensure the existing title can remain but do not rely on it alone
and keep data-theme-icon unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@public/error-reporter.9455923c.js`:
- Around line 92-93: The reported page URL currently sends full location.href
(which leaks query and hash) to the backend; update the error payload creation
where { url: location.href, resource: resourceUrl } is used (and the similar
occurrences around the other listed locations) to instead send a stripped URL
built from the location object (e.g., origin + pathname) or by constructing a
URL and omitting search and hash so query strings and fragments are removed
before forwarding to Slack; ensure you update every occurrence (the one shown
and the other similar blocks at the referenced ranges) so all reported urls are
sanitized.
- Around line 87-93: The code is reporting every IMG load failure, including
images with inline self-healing fallbacks; update the conditional before calling
reportError in the error reporter so IMG errors with inline handlers or explicit
opt-outs are ignored: specifically, when target.tagName === 'IMG' skip reporting
if target.hasAttribute('onerror') or target.hasAttribute('data-ignore-error')
(or similar attribute you add), otherwise preserve reporting for SCRIPT/LINK and
other IMG cases; update the branch around resourceUrl/target.tagName and
reference the reportError call to ensure only unhandled resource failures are
sent.

In `@public/index.html`:
- Around line 3842-3856: The SW purge runs concurrently with data loads, risking
stale cached responses; wrap the localhost purge (the
navigator.serviceWorker.getRegistrations() unregister sequence and caches.keys()
deletes) in an immediately-invoked async function, await the Promise.all results
for unregistration and cache deletion, and ensure that this awaited purge
completes before invoking the data loaders loadAuthState, loadRateLimit,
loadLatestRelease, loadRepos, loadAuthors, and loadPrs (i.e., move/await the
purge block so the loaders run only after the purge finishes).
- Line 31: Remove the defer attribute from the error-reporter script tag so its
global handlers install before bootstrap; specifically update the <script
src="error-reporter.9455923c.js"> inclusion (the file that registers global
error handlers used to capture startup errors from functions like
loadAuthState() and loadRepos()) to load immediately in the head rather than
deferred, ensuring the error-reporter executes before any inline initialization
code.

---

Duplicate comments:
In `@public/diagnostics.html`:
- Around line 58-61: The diagnostics theme toggle (element with
data-theme-toggle and its icon data-theme-icon) is an icon-only button without
an accessible name; update it to match the other toggle pattern by adding an
explicit accessible label (for example add a visually-hidden <span> with the
text "Toggle dark/light mode" inside the button or add aria-label="Toggle
dark/light mode" on the button) so screen readers get a reliable name; ensure
the existing title can remain but do not rely on it alone and keep
data-theme-icon unchanged.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository: OWASP-BLT/coderabbit/.coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: 67b9abd5-7f39-4d45-8419-d821370da0fc

📥 Commits

Reviewing files that changed from the base of the PR and between 6ff0a55 and add8e61.

📒 Files selected for processing (7)
  • public/asset-manifest.json
  • public/diagnostics.html
  • public/error-reporter.9455923c.js
  • public/error-reporter.js
  • public/how-it-works.html
  • public/index.html
  • public/test-error.html
✅ Files skipped from review due to trivial changes (2)
  • public/asset-manifest.json
  • public/test-error.html
🚧 Files skipped from review as they are similar to previous changes (2)
  • public/error-reporter.js
  • public/how-it-works.html

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (3)
public/error-reporter.9455923c.js (1)

60-64: Consider using a Blob with explicit Content-Type for sendBeacon.

navigator.sendBeacon(url, body) with a string sends it as text/plain, not application/json. While the backend's request.json() may still parse the body correctly, using a Blob ensures the correct Content-Type header is sent.

♻️ Optional fix for explicit Content-Type
             var ok = false;
             try {
-                ok = navigator.sendBeacon('/api/client-error', body);
+                ok = navigator.sendBeacon('/api/client-error', new Blob([body], { type: 'application/json' }));
             } catch (e) {
                 ok = false;
             }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@public/error-reporter.9455923c.js` around lines 60 - 64, The sendBeacon call
currently sends a string (body) which defaults to text/plain; update the
navigator.sendBeacon usage in the try block to send a Blob with explicit
Content-Type 'application/json' (e.g., new Blob([JSON.stringify(body)], { type:
'application/json' })) so the backend receives application/json; keep the
existing try/catch and assignment to ok, replacing the second argument to
navigator.sendBeacon with the JSON Blob while ensuring body is JSON-serializable
before creating the Blob.
public/index.html (2)

3940-3940: Verify the theme script path is correct.

The script source is theme.196e3e21.js (no leading /), making it relative to the current page path. If this page is accessed at a non-root path (e.g., /app/index.html), the browser would try to load /app/theme.196e3e21.js instead of /theme.196e3e21.js.

The error-reporter at line 31 uses an absolute path (/error-reporter.9455923c.js). Consider using the same pattern for consistency:

♻️ Use absolute path for theme script
-    <script src="theme.196e3e21.js"></script>
+    <script src="/theme.196e3e21.js"></script>
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@public/index.html` at line 3940, The theme script tag uses a relative path
("theme.196e3e21.js") which can break on non-root URLs; change the src to an
absolute path (e.g., "/theme.196e3e21.js") to match the error-reporter pattern
and ensure the browser always loads the correct asset; locate the <script>
element referencing "theme.196e3e21.js" in public/index.html and update its src
to the absolute path.

3856-3860: window.updateThemeIcons is not exported from theme.js — this call is dead code.

Per context snippet 4 from public/theme.js, only toggleTheme is exported to window:

window.toggleTheme = toggleTheme;

The updateThemeIcons function is not exported, so the typeof check at line 3857 always fails and the call never executes. This is harmless since theme.js calls updateThemeIcons() internally via its own DOMContentLoaded listener, but the code here is misleading.

Either export updateThemeIcons from theme.js or remove this dead code:

♻️ Option A: Remove dead code from bootstrapPageData
         (async function bootstrapPageData() {
-            if (typeof window.updateThemeIcons === 'function') {
-                window.updateThemeIcons();
-            }
-
             await setupServiceWorkerForEnvironment();
♻️ Option B: Export updateThemeIcons in theme.js

Add to public/theme.js:

 // Export for other scripts
 window.toggleTheme = toggleTheme;
+window.updateThemeIcons = updateThemeIcons;
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@public/index.html` around lines 3856 - 3860, The bootstrapPageData IIFE
contains a dead check/call for window.updateThemeIcons
(window.updateThemeIcons() in bootstrapPageData) but theme.js only exports
window.toggleTheme; either remove the dead code from bootstrapPageData (delete
the typeof check and call to window.updateThemeIcons) or export updateThemeIcons
from theme.js by adding window.updateThemeIcons = updateThemeIcons alongside
window.toggleTheme; update the file that defines bootstrapPageData (remove the
check/call) or update theme.js (add the export) so the symbols bootstrapPageData
and updateThemeIcons are consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@public/error-reporter.9455923c.js`:
- Around line 60-64: The sendBeacon call currently sends a string (body) which
defaults to text/plain; update the navigator.sendBeacon usage in the try block
to send a Blob with explicit Content-Type 'application/json' (e.g., new
Blob([JSON.stringify(body)], { type: 'application/json' })) so the backend
receives application/json; keep the existing try/catch and assignment to ok,
replacing the second argument to navigator.sendBeacon with the JSON Blob while
ensuring body is JSON-serializable before creating the Blob.

In `@public/index.html`:
- Line 3940: The theme script tag uses a relative path ("theme.196e3e21.js")
which can break on non-root URLs; change the src to an absolute path (e.g.,
"/theme.196e3e21.js") to match the error-reporter pattern and ensure the browser
always loads the correct asset; locate the <script> element referencing
"theme.196e3e21.js" in public/index.html and update its src to the absolute
path.
- Around line 3856-3860: The bootstrapPageData IIFE contains a dead check/call
for window.updateThemeIcons (window.updateThemeIcons() in bootstrapPageData) but
theme.js only exports window.toggleTheme; either remove the dead code from
bootstrapPageData (delete the typeof check and call to window.updateThemeIcons)
or export updateThemeIcons from theme.js by adding window.updateThemeIcons =
updateThemeIcons alongside window.toggleTheme; update the file that defines
bootstrapPageData (remove the check/call) or update theme.js (add the export) so
the symbols bootstrapPageData and updateThemeIcons are consistent.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository: OWASP-BLT/coderabbit/.coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: 40d1e8e6-9a50-4241-8d7e-6d0ee27d3d1d

📥 Commits

Reviewing files that changed from the base of the PR and between add8e61 and ce579e1.

📒 Files selected for processing (3)
  • public/error-reporter.9455923c.js
  • public/error-reporter.js
  • public/index.html
🚧 Files skipped from review as they are similar to previous changes (1)
  • public/error-reporter.js

@Rudra-rps
Copy link
Copy Markdown
Contributor Author

@DonnieBLT Ready..

@Tejas-Ladhani
Copy link
Copy Markdown

solves #290 & #291

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (2)
public/diagnostics.html (1)

171-177: Consider adding null checks for defensive coding.

If the data attributes are ever removed or renamed, themeToggle.addEventListener would throw. A null check would make this more resilient:

🛡️ Optional defensive check
 const themeToggle = document.querySelector('[data-theme-toggle]');
 const themeIcon = document.querySelector('[data-theme-icon]');
+if (themeToggle && themeIcon) {
 themeToggle.addEventListener('click', () => {
     const isDark = document.documentElement.classList.toggle('dark');
     themeIcon.className = isDark ? 'fas fa-sun' : 'fas fa-moon';
     localStorage.setItem('theme', isDark ? 'dark' : 'light');
 });
 // Set initial icon
 if (document.documentElement.classList.contains('dark')) {
     themeIcon.className = 'fas fa-sun';
 }
+}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@public/diagnostics.html` around lines 171 - 177, The event handler assumes
querySelector returned elements; add null checks before using them: verify
themeToggle is non-null before calling themeToggle.addEventListener and verify
themeIcon exists (or use optional chaining) before setting themeIcon.className
so the script won’t throw if the data attributes are missing or renamed; keep
the same behavior (toggle document.documentElement.classList, update
localStorage) inside the guarded block.
public/error-reporter.1b9dc33f.js (1)

170-175: Non-Error console.error calls may still generate noise.

The else branch reports all console.error calls that don't contain an Error instance. This could forward diagnostic/debug logs to Slack (e.g., console.error("Debug:", someObject)), potentially causing alert fatigue.

Consider gating this on a more restrictive condition, or adding a source marker that the backend can filter differently.

💡 Optional: only report non-Error calls if they look like real errors
             } else if (msg) {
-                reportError('ConsoleError', msg, msg, {
+                // Only report if message looks like an error, not debug output
+                var looksLikeError = /error|exception|fail|crash/i.test(msg);
+                if (looksLikeError) {
+                    reportError('ConsoleError', msg, msg, {
                         url: getPageUrl(),
-                        source: 'console.error',
-                });
+                        source: 'console.error:filtered',
+                    });
+                }
             }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@public/error-reporter.1b9dc33f.js` around lines 170 - 175, The current else
branch sends every non-Error console.error to reportError which causes noise;
update the handling in the console.error branch that calls
reportError('ConsoleError', msg, msg, { url: getPageUrl(), source:
'console.error' }) to either (A) gate non-Error reports with a conservative
heuristic (e.g., only report when typeof msg === 'string' &&
/error|failed|exception/i.test(msg') or when an additional context flag is
present) or (B) mark them so the backend can filter (e.g., change the metadata
to source: 'console.error:non-error' or add { nonError: true }) and only send
full alerts for true Error instances; apply this change where reportError and
getPageUrl are invoked.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@public/diagnostics.html`:
- Around line 171-177: The event handler assumes querySelector returned
elements; add null checks before using them: verify themeToggle is non-null
before calling themeToggle.addEventListener and verify themeIcon exists (or use
optional chaining) before setting themeIcon.className so the script won’t throw
if the data attributes are missing or renamed; keep the same behavior (toggle
document.documentElement.classList, update localStorage) inside the guarded
block.

In `@public/error-reporter.1b9dc33f.js`:
- Around line 170-175: The current else branch sends every non-Error
console.error to reportError which causes noise; update the handling in the
console.error branch that calls reportError('ConsoleError', msg, msg, { url:
getPageUrl(), source: 'console.error' }) to either (A) gate non-Error reports
with a conservative heuristic (e.g., only report when typeof msg === 'string' &&
/error|failed|exception/i.test(msg') or when an additional context flag is
present) or (B) mark them so the backend can filter (e.g., change the metadata
to source: 'console.error:non-error' or add { nonError: true }) and only send
full alerts for true Error instances; apply this change where reportError and
getPageUrl are invoked.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 098c4a5f-e166-4eb0-a9c5-f426b23c63ac

📥 Commits

Reviewing files that changed from the base of the PR and between ce579e1 and b09c98e.

📒 Files selected for processing (8)
  • public/asset-manifest.json
  • public/diagnostics.html
  • public/error-reporter.1b9dc33f.js
  • public/error-reporter.js
  • public/how-it-works.html
  • public/index.html
  • public/test-error.html
  • src/index.py
✅ Files skipped from review due to trivial changes (2)
  • public/asset-manifest.json
  • public/error-reporter.js
🚧 Files skipped from review as they are similar to previous changes (3)
  • public/test-error.html
  • public/how-it-works.html
  • public/index.html

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants